Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow blinded path diversification by expanding create_blinded_paths #3087

Merged
merged 3 commits into from
Jul 16, 2024

Conversation

shaavan
Copy link
Contributor

@shaavan shaavan commented May 31, 2024

  • The current usage of blinded_paths is limited because create_blinded_path only returns a single BlindedPath.
  • This PR expands the functionality of create_blinded_path by allowing it to return multiple BlindedPaths, as determined by the new count parameter.
  • Additionally, this PR integrates this new capability throughout the codebase by:
    • Allowing multiple paths in offers and refund builders.
    • Sending Offers Response messages, such as InvoiceRequest (in pay_for_offer) and Invoice (in request_refund_payment), using multiple reply paths.
  • As a proof-of-concept, this PR increases the maximum count of create_blinded_paths to 10, enabling the generation of more reply paths. It also increases the number of blinded_paths used in offer and refund builders and responders to 5, demonstrating the usage of multiple reply paths.

@jkczyz jkczyz self-requested a review May 31, 2024 19:56
@codecov-commenter
Copy link

codecov-commenter commented May 31, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.78%. Comparing base (78c0eaa) to head (957b337).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3087      +/-   ##
==========================================
- Coverage   89.80%   89.78%   -0.02%     
==========================================
  Files         121      121              
  Lines      100045   100207     +162     
  Branches   100045   100207     +162     
==========================================
+ Hits        89845    89975     +130     
- Misses       7533     7558      +25     
- Partials     2667     2674       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lightning/src/onion_message/messenger.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
lightning/src/offers/refund.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
@shaavan
Copy link
Contributor Author

shaavan commented Jun 3, 2024

Updated from pr3087.01 to pr3087.02 (diff):
Addressed @jkczyz comment

Changes:

  1. Remove the redundant MAX_PATHS constant and the associated debug_assert!
  2. Reduce the number of paths created in offers, and refund builder to 1, and introduce a TODO comment.
  3. Refactor the path functions
  4. Update pay_for_offer to allow setting REQUEST_LIMIT to the number of messages created, instead of the number of the path used. Also updated the comment accordingly.

lightning/src/ln/channelmanager.rs Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
@shaavan
Copy link
Contributor Author

shaavan commented Jun 6, 2024

Updated from pr3087.02 to pr3087.03 (diff):
Changes:

  1. Rebase on main.

@shaavan
Copy link
Contributor Author

shaavan commented Jun 6, 2024

Updated from pr3087.03 to pr3087.04 (diff):
Addressed @jkczyz comments

Changes:

  1. Move the and_then call to the call, and revert the Offer and Refund Builder path functions to the original. The function again adds only a single path to offer/refund paths.
  2. Refactored the pay_for_offer code to remove redundant clonings.
  3. Updated request_refund_payment to use similar REQUEST_LIMIT as pay_for_offer.
  4. Introduce a constant MAX_REPLY_PATHS to pay_for_offer and request_refund_payment.

@shaavan
Copy link
Contributor Author

shaavan commented Jun 10, 2024

Updated from pr3087.04 to pr3087.05 (diff):
Addressed @TheBlueMatt and @jkczyz

Updates:

  1. Reverted changes to MessageRouter's create_blinded_paths.
  2. Update the create_blinded_path function to return the entire vector returned by create_blinded_paths, and hence remove the count parameter.

@shaavan
Copy link
Contributor Author

shaavan commented Jun 10, 2024

Updated from pr3087.05 to pr3087.06 (diff):

Changes:

  1. Rebase on main.
  2. Also update create_blinded_path_using_absolute_expiry and create_compact_blinded_path to return Vec<BlindedPath>

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/onion_message/messenger.rs Outdated Show resolved Hide resolved
@shaavan
Copy link
Contributor Author

shaavan commented Jun 12, 2024

Updated from pr3087.06 to pr3087.07 (diff):
Addressed @jkczyz comment

Changes:

  1. Reverted the changes in onion_messenger.rs as they were not affecting the behavior.
  2. Updated function names and docs to better suit their roles.
  3. Remove the TODO comment as the exact interface is to be decided.

TheBlueMatt
TheBlueMatt previously approved these changes Jun 13, 2024
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jbesraa jbesraa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good
Is it worth adding a test to cover the new code?

@jkczyz
Copy link
Contributor

jkczyz commented Jun 20, 2024

Looks good Is it worth adding a test to cover the new code?

Chatted with @shaavan earlier today about how to go about testing this.

@shaavan
Copy link
Contributor Author

shaavan commented Jun 24, 2024

Updated from pr3087.07 to pr3087.08 (diff):
Addressed @TheBlueMatt, @jbesraa comment

Changes:

  1. Pulled REQUEST_LIMIT into a global const, and renamed it to OFFERS_MESSAGE_REQUEST_LIMIT.
  2. Introduce a test to check the new behavior.

Thanks, @jkczyz, for the pointers and inspirations for the testing.

TheBlueMatt
TheBlueMatt previously approved these changes Jun 28, 2024
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Show resolved Hide resolved
@shaavan
Copy link
Contributor Author

shaavan commented Jun 29, 2024

Updated from pr3087.09 to pr3087.10 (diff):
Addressed @jbesraa comments

Changes:

  1. Referenced the PaymentId in the const's documentation.

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
@shaavan
Copy link
Contributor Author

shaavan commented Jul 2, 2024

Updated from pr3087.10 to pr3087.11 (diff):
Addressed @jkczyz comments

Changes:

  1. Replace invoices -> requests to match the context the word is used.
  2. Remove the reference for PaymentId since it's already used in the file.

@shaavan
Copy link
Contributor Author

shaavan commented Jul 5, 2024

Updated from pr3087.11 to pr3087.12 (diff):
Addressed @jkczyz comment

Changes:

  1. Update extract_invoice() to also return the corresponding reply_path.
  2. Introduce reply_path_diversification test for refund case.

@shaavan
Copy link
Contributor Author

shaavan commented Jul 5, 2024

Updated from pr3087.12 to pr3087.13 (diff):

Changes:

  1. Rebase on main.
  2. Change the return type of extract_invoice() from (Bolt12Invoice, BlindedPath) -> (Bolt12Invoice, Option<BlindedPath>), to avoid some test failures.

lightning/src/ln/offers_tests.rs Show resolved Hide resolved
lightning/src/ln/offers_tests.rs Show resolved Hide resolved
lightning/src/ln/offers_tests.rs Outdated Show resolved Hide resolved
match node.onion_messenger.peel_onion_message(message) {
Ok(PeeledOnion::Receive(message, _, _)) => match message {
Ok(PeeledOnion::Receive(message, _, reply_path)) => match message {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a follow-up PR, could you start including a reply path with invoices for offers? We should have the capability now, but we just haven't updated ChannelManager's implementation of OffersMessageHandler yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! Added it to the task list! 🚀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
@shaavan
Copy link
Contributor Author

shaavan commented Jul 9, 2024

Updated from pr3087.13 to pr3087.14 (diff):
Addressed @jkczyz comments

Updates:

  1. Rename added test, to be more explicit.
  2. Update added const comment to be more clear.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good. I went through the tests and noted where we should remove checks that aren't relevant to the new behavior. There may be tests where we are asserting more than necessary, but might as well avoid doing that in newer tests.

lightning/src/ln/offers_tests.rs Outdated Show resolved Hide resolved
lightning/src/ln/offers_tests.rs Outdated Show resolved Hide resolved
lightning/src/ln/offers_tests.rs Outdated Show resolved Hide resolved
lightning/src/ln/offers_tests.rs Outdated Show resolved Hide resolved
lightning/src/ln/offers_tests.rs Outdated Show resolved Hide resolved
lightning/src/ln/offers_tests.rs Outdated Show resolved Hide resolved
…ication

- Previously, the `create_blinded_path` function was limited to
  returning a single `BlindedPath`, which restricted the usage of
  `blinded_paths`.
- This commit extends the `create_blinded_path` function to return
  the entire blinded path vector generated by the `MessageRouter`'s
  `create_blinded_paths`.
- The updated functionality is integrated across the codebase, enabling
  the sending of Offers Response messages, such as `InvoiceRequest`
  (in `pay_for_offer`) and `Invoice` (in `request_refund_payment`),
  utilizing multiple reply paths.
- This will be utilised in the following commit for a test.
@shaavan
Copy link
Contributor Author

shaavan commented Jul 11, 2024

Updated from pr3087.14 to pr3087.15 (diff):

Updates:

  1. Rebase on main to resolve merge conflicts.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please squash the fixup.

@shaavan
Copy link
Contributor Author

shaavan commented Jul 12, 2024

Updated from pr3087.16 to pr3087.17 (diff):
Addressed @jkczyz comment

  1. Squashed the f: commits together.
  2. Fixed one failing test by reintroducing an important line of code.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Diff since @jkczyz's "LGTM" is trivial, so landing:

$ git diff-tree -U2 e6bb965bcd3fede82b956c5abb17d6e501c8d9af 957b33712ad3d5a7aed624e4f565887817fd9944
diff --git a/lightning/src/ln/offers_tests.rs b/lightning/src/ln/offers_tests.rs
index 2eddb1991..cdd78d02c 100644
--- a/lightning/src/ln/offers_tests.rs
+++ b/lightning/src/ln/offers_tests.rs
@@ -1019,4 +1019,6 @@ fn send_invoice_for_refund_with_distinct_reply_path() {
 	expect_recent_payment!(alice, RecentPaymentDetails::AwaitingInvoice, payment_id);

+	let _expected_invoice = david.node.request_refund_payment(&refund).unwrap();
+
 	connect_peers(david, bob);
$ 

@TheBlueMatt TheBlueMatt merged commit 6ed398d into lightningdevkit:main Jul 16, 2024
13 of 17 checks passed
@shaavan shaavan deleted the reply_path_diversity branch July 17, 2024 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants